Skip to content

Conversation

@swillner
Copy link
Contributor

This PR builds on top of #141

It includes:

  • Options for submit_batch_job: "ntasks", "cpus_per_task", "min_cpus", "max_cpus", "min_nodes", max_nodes":
    • As I understand it, the submit_batch_job method is along the lines of the _fill_job_desc_from_opts function in src/sbatch/sbatch.c of the Slurm source. Accordingly I adjusted the options involving "ntasks_set" and "cpus_set" which are both pretty much set along with "ntasks" and "cpus_per_task" in the Slurm source.
    • However, "min_nodes" should be set from "nodelist" (as is done in sbatch). Though, I suspect the worst that could happen is that an inconsistent job is declined by Slurm.\
  • Fix of the db connection in slurmdb_jobs:
    • Use the class-scope db_conn and close when the slurmdb_jobs instance is freed.
    • Also free the job_cond which has been allocated in __cinit__.

@giovtorres Regarding the Python 2.6 tests: These seem to fail because of a missing "debug" partition in the CentOS 6 docker containers (newer Python versions use CentOS 7). Couldn't you just drop official Python 2.6 support?


def __dealloc__(self):
pass
self.__free()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since __free() is "private" and not used elsewhere, I think we can move the xfree and slurmdb_connection_close directly under __dealloc__.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem. i intended to follow the structure of some of the other classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ;)

@giovtorres
Copy link
Member

Thank you for this contribution!

I do need to drop support for Python 2.6. I tried about year ago or so and some folks asked me to hold off, but I think it's time. Also, Python 2.7 support is going away on Jan 1 and may projects have pledged to drop support for Python 2. I'm considering the same for PySlurm. Python 3 support has been working for over a year now. I want to drop support for that CentOS 6 container altogether.

# desc.min_cpus = job_opts.get("ntasks")

# TODO: ntasks_set, cpus_set
if job_opts.get("overcommit"):
Copy link
Member

@giovtorres giovtorres Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we work on some tests for these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submit_batch_job does not seem to be tested beyond giving a wrap script and a job_name. I added simple tests for ntasks and cpus_per_task...

test_job_id = pyslurm.job().submit_batch_job(test_job)
test_job_search = pyslurm.job().find(name="name", val="pyslurm_test_job")
assert_true(test_job_id in test_job_search)
assert_equals(test_job_search["cpus_per_task"], 3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@giovtorres giovtorres merged commit 3636e74 into PySlurm:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants